Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add TF/Pose_V pub in DiffDrive #548

Merged
merged 5 commits into from
Feb 23, 2021
Merged

add TF/Pose_V pub in DiffDrive #548

merged 5 commits into from
Feb 23, 2021

Conversation

doisyg
Copy link
Contributor

@doisyg doisyg commented Jan 12, 2021 •
edited
Loading

Solves gazebosim/ros_gz#131
With this PR, there is only /tf@tf2_msgs/[email protected]_V to add to the args of the ros_ign_bridge parameter_bridge node to get similar TF publishing behavior as in Gazebo Classic DiffDrive plugin.
Once merged I can PR ros_ign_gazebo_demos if needed with the additional /tf@tf2_msgs/[email protected]_V.

I did the most straightforward implementation. Improvement to discuss:

  • Variable naming: tfPub, tf_msg or pose_VPub and pose_V_msg ?
  • Ign topic for frame_id -> child_frame_id transform: tf or something else ?
  • TF/Pose_V always on, or enable/disable for instance through _sdf->HasElement("publish_tf") (or other name)

add TF/Pose_V pub in DiffDrive
0f48301 
Signed-off-by: Guillaume Doisy <[email protected]>
@doisyg doisyg requested a review from chapulina as a code owner January 12, 2021 12:47
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jan 12, 2021
@doisyg doisyg mentioned this pull request Jan 12, 2021
Closed
codecheck
423ac10 
Signed-off-by: Guillaume Doisy <[email protected]>
@chapulina
Copy link
Contributor

Thanks for the PR! Did you consider using the PosePublisher system alongside the DiffDrive?

@doisyg
Copy link
Contributor Author

doisyg commented Jan 16, 2021

I wasn't aware of the PosePublisher plugin but I guess that could work too!

@doisyg
Copy link
Contributor Author

doisyg commented Jan 16, 2021

Though if it is the plugin that publishes directly the Pose_V / tf, we get a behavior/configuration more similar to the gazebo classic in my opinion.

@chapulina
Copy link
Contributor

if it is the plugin that publishes directly the Pose_V / tf, we get a behavior/configuration more similar to the gazebo classic in my opinion.

Agreed, but in the long run, less duplication is easier to maintain. If we need to update something about how transforms are being published, right now we only need to update the PosePublisher and all users benefit from it. I'm inclined to suggest users migrating from Gazebo classic use the 2 plugins, instead of duplicating the functionality here.

@doisyg
Copy link
Contributor Author

doisyg commented Feb 1, 2021

I gave it a try but I fail to understand how to configure PosePublisher to publish the transform from odom to the link of the robot.
What model/link would be odom ?

@doisyg
Copy link
Contributor Author

doisyg commented Feb 15, 2021

Coming back on this, I cannot find a solution to get the tf published from frame_id to child_frame_id without this PR and by using the PosePublisher. Would you reconsider the merge ?

chapulina
chapulina reviewed Feb 16, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What model/link would be odom ?

Good question. I can see now how this PR is needed. Besides addressing the minor comments below, do you mind also documenting the new topic in DiffDrive.hh? Thanks!

src/systems/diff_drive/DiffDrive.cc Outdated Show resolved Hide resolved
src/systems/diff_drive/DiffDrive.cc Outdated Show resolved Hide resolved
chapulina
chapulina approved these changes Feb 19, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for iterating, looks good to me. I just have one final question about the test.

test/integration/diff_drive_system.cc Show resolved Hide resolved
missing pub in DiffDrive tests
1117f1c 
Signed-off-by: Guillaume Doisy <[email protected]>
line length
c4fda6a 
Signed-off-by: Guillaume Doisy <[email protected]>
@chapulina chapulina merged commit 60d9eb8 into gazebosim:ign-gazebo3 Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

Assignees
No one assigned
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@doisyg @chapulina